-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Small Tags performance improvements #6185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a268978 to
1e8788f
Compare
| if (!current.equals(next)) | ||
| tags[j++] = tags[i]; | ||
| current = next; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've extracted dedup to a separate method to see that in isolation i can reduce ~1000 ns to ~750 ns on workstation by this (not sure how many elements i was feeding in, either 64 or 128). I haven't tested explicitly the same above, but both can be dropped if you feel this is too much.
micrometer-core/src/main/java/io/micrometer/core/instrument/Tags.java
Outdated
Show resolved
Hide resolved
| } | ||
| return toTags(tagsCollection.toArray(EMPTY_TAG_ARRAY)); | ||
| } | ||
| else if (emptyIterable(tags)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emptyIterable was used more widely in a version that hasn't reached publication, maybe we want to get rid of it completely. There are some repeated checks, compiler should be able to pick up invariants during inlining, but i've never explicitly checked.
| return Tags.empty(); | ||
| } | ||
| else if (tags instanceof Tags) { | ||
| if (tags instanceof Tags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since EMPTY is instanceof Tags, it will fall in this branch as well. Instanceof requires one extra memory lookup though, so idk.
Null is also recognized only at the emptyIterable stage, but this shouldn't bring additional cost, i don't expect JIT to try to check for null thrice
| if (length == 0) { | ||
| return other; | ||
| } | ||
| if (Objects.equals(this, other)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.equals is basically a null-safe version of calling equals directly, and the first thing this code was doing is an access to other that isn't possible in the null case.
1e8788f to
a574645
Compare
Signed-off-by: etki <[email protected]>
a574645 to
c28c6a6
Compare
This is the tiniest performance bump born out of the same story as #6113. There are few changes that benefit insignificantly (single digit percent) - it's completely OK to say "we'd want to keep things as we have here" (i'm only having fun and getting some extra experience here). It's easier to annotate specific code parts, so you will find individual comments below.
The performance was evaluated with benchmarks from #6174, OpenJDK 21.0.2 and Intel N100 fixed at 2GHz. Since there is different processing for different subtypes of Iterable, a parameter matrix with different distributions of these implementations is introduced, when category is
absent, then all implementations are uniformly distributed, and when category is X, it means that 90% of input is X, and the rest is evenly divided between other implementations. Every instance contains 0-68 tags (a uniformly selected number), with the exception of null. Please also note thatsetuses HashSet, which should be slower than JDK9+ Set.of(). What is used by the real clients more (Set.of() or HashSet) is unknown to me.Concat: common case, any implementation equiprobable, all standard counters
Concat: matrix
3453.615 ± 12.3113332.854 ± 15.0921553.365 ± 8.6111503.860 ± 7.0852680.919 ± 15.9522627.852 ± 12.5363257.486 ± 8.4963125.463 ± 14.7897297.398 ± 12.9897078.009 ± 6.8433427.177 ± 18.2063313.541 ± 7.1301476.249 ± 2.3491438.450 ± 2.499194.101 ± 1.208193.595 ± 1.029310.539 ± 1.333294.154 ± 1.905838.107 ± 1.863843.229 ± 2.5464835.388 ± 4.7004741.637 ± 11.5171477.644 ± 3.6761421.480 ± 3.9332452.261 ± 13.2772398.422 ± 12.465335.398 ± 2.342310.834 ± 2.3991515.396 ± 3.0101473.454 ± 7.2792111.102 ± 6.9312019.852 ± 2.9216109.320 ± 13.1585985.968 ± 11.3792458.731 ± 5.3632414.234 ± 11.0443033.881 ± 4.7373003.112 ± 14.427845.153 ± 2.803849.731 ± 1.8052067.637 ± 2.2472023.014 ± 5.0552634.236 ± 5.4562586.595 ± 5.5086802.838 ± 10.7066557.061 ± 16.5763054.109 ± 7.3442915.368 ± 8.8657288.231 ± 11.3027101.247 ± 16.2654811.101 ± 8.8124657.866 ± 4.6886104.217 ± 7.1435942.757 ± 11.1246679.737 ± 8.9776518.054 ± 12.78010592.943 ± 24.44410396.221 ± 19.1247270.077 ± 6.2107126.497 ± 23.7513440.212 ± 13.3193315.371 ± 10.6431568.332 ± 7.1861490.758 ± 3.8352664.903 ± 6.2522562.616 ± 15.8443212.028 ± 6.3833151.855 ± 14.4857238.005 ± 14.1857120.775 ± 18.2573493.186 ± 11.9783303.420 ± 11.901Of: common case, all implementations equiprobable, standard counters
Of: distributions
Not sure what's going on with lsit here. Needs just a more more detailed look, but i'm not sure if i'll ever have the time.
1252.774 ± 2.2281212.606 ± 3.81073.245 ± 0.45370.854 ± 0.37688.168 ± 0.34369.111 ± 0.371589.633 ± 1.413603.960 ± 1.7534578.833 ± 9.5014451.159 ± 9.0311255.498 ± 3.8671226.269 ± 4.025